Preserve HTTPS angle-bracket autolinks in safe-output sanitization#48169
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Preserves HTTPS/CommonMark and Slack mrkdwn autolinks during safe-output sanitization while retaining domain filtering.
Changes:
- Preserves angle-bracket HTTPS links.
- Adds domain handling for labeled autolinks.
- Adds sanitizer and ingestion regressions.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/sanitize_content_core.cjs |
Preserves and filters HTTPS autolinks. |
actions/setup/js/sanitize_content.test.cjs |
Tests sanitizer behavior. |
actions/setup/js/collect_ndjson_output.test.cjs |
Tests custom safe-job ingestion. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Medium
| * @returns {boolean} | ||
| */ | ||
| function isHttpsAngleBracketAutolink(tagContent) { | ||
| return /^https:\/\/[^\s<>|]+(?:\|[^<>]*)?$/i.test(tagContent); |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
Thanks for the fix! 🎉 This PR looks great — it clearly solves issue #47643 by teaching the sanitizer to recognize and preserve HTTPS angle-bracket autolinks (both plain and Slack mrkdwn forms) while maintaining URL domain filtering. What's working well:
This is ready for review and merge. Great work!
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (4 tests)
Quality Highlights✨ Excellent test design:
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Preserve HTTPS angle-bracket autolinks in safe-output sanitization
The fix is correct and well-structured — isHttpsAngleBracketAutolink guards convertXmlTags before tag rewriting, and the first-pass replacement in sanitizeUrlDomains handles Slack mrkdwn links as a unit. Two non-blocking issues found:
- Path regex excludes
|too eagerly (line 269):[^\s<>|]*stops at|, so URLs with|in query strings are mis-parsed. Use[^\s<>]*and split the Slack label at the last|. - Missing test for disallowed Slack-style links: The label text is silently dropped when the domain is blocked. A test should pin this behavior.
No security or blocking correctness issues found beyond the existing IPv6 comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 30.1 AIC · ⌖ 5.73 AIC · ⊞ 5K
| * @returns {string} The string with unknown domains redacted | ||
| */ | ||
| function sanitizeUrlDomains(s, allowed) { | ||
| const angleBracketHttpsAutolinkRegex = /<https:\/\/([\w.-]+(?::\d+)?)(\/[^\s<>|]*)?(?:\|([^<>]*))?>/gi; |
There was a problem hiding this comment.
The path capture group [^\s<>|]* stops at |, so a URL with a literal | in its query string (e.g. (example.com/redacted)) is parsed incorrectly — the path is truncated and the remainder treated as a Slack label. While bare | in URLs is uncommon, it is valid per RFC 3986. Consider capturing the path as [^\s<>]* and splitting off the Slack label only at the last |, or document this known limitation.
@copilot please address this.
|
|
||
| it("should handle CDATA sections", () => { | ||
| const result = sanitizeContent("<![CDATA[<script>alert('xss')</script>]]>"); | ||
| expect(result).toBe("(![CDATA[(script)alert('xss')(/script)]])"); |
There was a problem hiding this comment.
There is no test covering a Slack mrkdwn link whose domain is not in the allowlist (e.g. (evil.com/redacted)). The current redaction path in sanitizeUrlDomains silently drops the label text when the domain is blocked — the output is (evil.com/redacted) rather than something like (evil.com/redacted|click here). Please add a test to pin the expected output for disallowed Slack-style links so regressions are caught.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on one correctness defect and its missing regression test.
📋 Key Themes & Highlights
Key Issues
- Label-loss bug (
sanitize_content_core.cjsline 328): when a Slack mrkdwn link contains a disallowed domain,applyDomainFilterreturns(hostname/redacted)and the callback returns that bare string, silently discarding the angle brackets and the|labeltext entirely. - Missing test: the new test suite covers the happy path (allowed domain) but has no case for a disallowed domain in a Slack link, so the bug above is undetected.
Positive Highlights
- ✅ Root-cause fix is well-targeted:
isHttpsAngleBracketAutolinkcorrectly gates only HTTPS forms. - ✅ End-to-end integration test in
collect_ndjson_output.test.cjsis a good addition. - ✅ The two-pass strategy (handle angle-bracket links before generic URL matching) is the right approach to avoid greedy regex overlap.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 35.4 AIC · ⌖ 4.8 AIC · ⊞ 6.7K
Comment /matt to run again
| s = s.replace(angleBracketHttpsAutolinkRegex, (match, hostnameWithPort, path = "") => { | ||
| const url = `https://${hostnameWithPort}${path || ""}`; | ||
| const filtered = applyDomainFilter(url, hostnameWithPort); | ||
| return filtered === url ? match : filtered; |
There was a problem hiding this comment.
[/diagnosing-bugs] Slack label is silently dropped when the domain is disallowed. When applyDomainFilter redacts a disallowed domain, filtered becomes "(hostname/redacted)" — the outer angle brackets and |label text are lost entirely.
💡 Fix: reconstruct the full Slack link on redaction
The callback receives the full match (e.g. (evil.com/redacted)) and the label via capture group 3. When the domain is filtered you should reconstruct an appropriate replacement that preserves the label:
s = s.replace(angleBracketHttpsAutolinkRegex, (match, hostnameWithPort, path = "", label) => {
const url = `https://${hostnameWithPort}${path || ""}`;
const filtered = applyDomainFilter(url, hostnameWithPort);
if (filtered === url) return match; // allowed — preserve original angle-bracket form
// Disallowed — redact URL but keep label if present
return label ? `${filtered}|${label}` : filtered;
});Without this, a message like (evil.com/redacted) silently becomes (evil.com/redacted), discarding the visible label text the author intended.
@copilot please address this.
| it("should preserve Slack mrkdwn links on allowed HTTPS domains", () => { | ||
| const input = "Tracking issue: <https://github.com/octo-org/octo-repo/issues/123|Build failure — Build github/gh-aw#456>"; | ||
| expect(sanitizeContent(input)).toBe(input); | ||
| }); |
There was a problem hiding this comment.
[/tdd] No test covers the disallowed-domain path for Slack mrkdwn links — the label-loss defect (line 328 in sanitize_content_core.cjs) is invisible without it.
💡 Add a companion test for the disallowed-domain case
it("should redact disallowed domain in Slack mrkdwn link while preserving label", () => {
const input = "See: (evil.example.com/redacted)";
const result = sanitizeContent(input);
// URL should be redacted but label text retained
expect(result).not.toContain("evil.example.com");
expect(result).toContain("Build failure");
});Without this test the current code silently discards Build failure, and a future refactor has no safety net.
@copilot please address this.
There was a problem hiding this comment.
Non-blocking observations — the fix is correct and the two-pass approach (preserve in convertXmlTags, then domain-filter in sanitizeUrlDomains) is sound.
### Review findings
Medium — regex allocation per call
angleBracketHttpsAutolinkRegex is defined inside sanitizeUrlDomains, so a new RegExp object is allocated on every invocation. Hoist it to module-level (same pattern as httpsUrlRegex). See inline comment.
Medium — no redaction test for blocked-domain Slack links
The new tests cover the happy path (github.com allowed) but nothing asserts that (evil.com/redacted) gets redacted and its label is dropped. The silent label-drop behavior is also undocumented. See inline comment.
Low — duplicate "First pass" comment labels
Both passes in sanitizeUrlDomains are labeled "First pass", making execution order unclear. See inline comment.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 70.7 AIC · ⌖ 6.44 AIC · ⊞ 5.7K
Comment /review to run again
| * @returns {string} The string with unknown domains redacted | ||
| */ | ||
| function sanitizeUrlDomains(s, allowed) { | ||
| const angleBracketHttpsAutolinkRegex = /<https:\/\/([\w.-]+(?::\d+)?)(\/[^\s<>|]*)?(?:\|([^<>]*))?>/gi; |
There was a problem hiding this comment.
Regex recompiled on every sanitizeUrlDomains call: angleBracketHttpsAutolinkRegex is defined inside the function, creating a new RegExp object on every invocation.
💡 Suggested fix
Hoist it to module-level alongside the other URL regexes (httpsUrlRegex, etc.) already defined at file scope:
// module-level, near the other URL-related regexes
const angleBracketHttpsAutolinkRegex = /<https:\/\/(\w[\w.-]*(?::\d+)?)(\/[^\s<>|]*)?(?:\|([^<>]*))?>/gi;String.prototype.replace resets lastIndex automatically so hoisting with the g flag is safe here.
sanitizeUrlDomains is called per non-code region on every agent output string; allocating a new regex object each time is unnecessary overhead.
|
|
||
| // First pass: handle angle-bracket autolinks and Slack mrkdwn links as a unit so | ||
| // later generic URL matching does not consume the trailing ">" or "|label". | ||
| s = s.replace(angleBracketHttpsAutolinkRegex, (match, hostnameWithPort, path = "") => { |
There was a problem hiding this comment.
Disallowed-domain Slack mrkdwn links have no test coverage for redaction: The new angle-bracket first pass handles blocked domains, but there is no test asserting that (evil.com/redacted) is redacted and its label is dropped.
💡 Suggested test addition
Add a test in sanitize_content.test.cjs that verifies a disallowed-domain Slack-style link is redacted:
it("should redact Slack mrkdwn links with disallowed domains", () => {
const input = "See (evil.com/redacted)";
const result = sanitizeContent(input);
expect(result).not.toContain("evil.com");
expect(result).not.toContain("Click here"); // label is lost on redaction — document this
});Without this, a regression that accidentally passes the label through for blocked domains would be undetected. The current behavior (silently dropping the label when the domain is blocked) is also worth a comment since it's a non-obvious behavioral contract.
| return filtered === url ? match : filtered; | ||
| }); | ||
|
|
||
| // First pass: handle explicit https:// URLs |
There was a problem hiding this comment.
Duplicate "First pass" comment label: Both the angle-bracket pass (line 323) and the generic HTTPS pass (line 331) are labeled "First pass", making the ordering confusing to readers.
💡 Suggested fix
Rename the comments to reflect their actual execution order:
// Pass 1: handle angle-bracket autolinks and Slack mrkdwn links as a unit ...
s = s.replace(angleBracketHttpsAutolinkRegex, ...);
// Pass 2: handle remaining explicit https:// URLs
s = s.replace(httpsUrlRegex, ...);
|
@copilot please run the Unresolved review feedback:
Please cover both code and test follow-up in one pass.
|
|
@copilot add tests in fuzz tests for parser |
Current state verified: Reviews — The 4 referenced review signals (
Checks — All 6 completed CI runs show Mergeable — No further code changes are needed. Ready for maintainer action:
|
🤖 PR Triage — §30214847023
Rationale: HTTPS angle-bracket autolink preservation in safe-output sanitization. Low-risk: 139 additions focused on test files + sanitize_content_core.cjs. CI passing (copilot: success). Multiple AI reviewers approved. Well-tested with fuzz test added. Ready for expedited human review.
|
|
@copilot please run the skill, review the current branch state, address any still-open reviewer feedback and failed checks, and reply with whether this PR is ready for maintainer review. Run: https://github.com/github/gh-aw/actions/runs/30215321924
|
Current branch state verified — no further code changes are required. Local validation:
Review threads: All 8 threads are addressed in commits
PR status:
Hand-off for maintainers:
|
No further code changes are required. All 8 listed threads are addressed in the current HEAD (
6 threads are Remaining for maintainers:
|
🤖 PR Triage
Summary: Safe-output sanitizer incorrectly rewrites Slack/CommonMark HTTPS autolinks Next action: Resolve any outstanding review comments, then fast-track.
|
No further code changes are required. All 8 listed threads are fully addressed in the current HEAD ( Thread status:
6 of 8 threads are What blocks merge (maintainer action required):
|
🤖 PR Triage
Score breakdown: Impact 32/50 · Urgency 18/30 · Quality 15/20 Rationale: Slack/CommonMark autolinks (
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. |
|
🎉 This pull request is included in a new release. Release: |
Custom safe-output job string inputs were sanitizing Slack/CommonMark autolinks like
<https://...|label>as if they were unknown HTML tags, rewriting them to parentheses before they reached downstream sinks. This preserves those HTTPS angle-bracket links while keeping existing URL allowlist filtering in place.Sanitizer behavior
convertXmlTagsto recognize HTTPS angle-bracket autolinks and leave them intact instead of treating them as XML/HTML.<https://example.com><https://example.com|label>URL policy preservation
Regression coverage
<url|label>) in custom safe-job string inputs #47643run: https://github.com/github/gh-aw/actions/runs/30207935610
Run: https://github.com/github/gh-aw/actions/runs/30218624846